New rule: Favour Nested Functions #648
Conversation
webwarrior-ws
force-pushed
the
favour-nested-functions
branch
from
2857119 to
8d130ab
Compare
xperiandri
left a comment
xperiandri
left a comment
There was a problem hiding this comment.
Looks good, just a few remarks
|
Could you rebase and fix? |
webwarrior-ws
force-pushed
the
favour-nested-functions
branch
3 times, most recently
from
0de6e84 to
bb6471e
Compare
|
@webwarrior-ws let's rebase this, which should fix the CI to be green. |
Added FavourNestedFunctions rule and tests for it.
Implemented FavourNestedFunctions rule. Added rule text message to Text.resx. Fixes fsprojects#638
webwarrior-ws
force-pushed
the
favour-nested-functions
branch
from
08a13a0 to
904ef9c
Compare
knocte
changed the title
Updated Configuration.fs and fsharplint.json to include FavourNestedFunctions rule. Added docs.
webwarrior-ws
force-pushed
the
favour-nested-functions
branch
from
6397f05 to
acf70ec
Compare
|
@webwarrior-ws let's fix SelfCheck |
|
Damn, I just realised that this rule is also finding Literal values, which is not exactly bad, but then it means that the rule name is not the best: should we rename from FavourNestedFunctions to FavourLocalNestedOverPrivate? |
|
Sometimes nesting functions will make the function too big. What to do in such cases? Remove |
Literals can't be nested inside functions. Only module-level. |
Then that's a false positive to fix before merging. Anyway, my point still stands wrt module elements that are not functions, just fields. |
Remove private? Then the rule would be forcing you to expose your internals!!
Sigh, we're not applying a rule suggestion to ignore another rule. Ignoring rules should be done in very specific scenarios where it cannot be avoided but in this case it looks like it would need too many ignores; therefore the proper way to fix this is tweak MaxLinesInFunction to not count nested function lines |
That test recursive function definitions.
Fixed bug when rule was not correctly identifying functions when they are declared using let rec ... and ... syntax.
|
Discovered another problem: |
webwarrior-ws
force-pushed
the
favour-nested-functions
branch
from
ed0eeec to
97ee66d
Compare
That makes sure that literals are not triggering the rule.
Do not fire the rule for bindings with [<Literal>] attribute because they can only be module-level.
webwarrior-ws
force-pushed
the
favour-nested-functions
branch
from
97ee66d to
0c5a4c3
Compare
So maybe don't apply the rule if the function has any attributes? |
Right, let's add a test for it. |
That makes sure that functions with attributes are not triggering the rule.
Because using attributes on nested functions (e.g [<TailCall>]) will give syntax error: ``` Unexpected symbol '[<' in expression ```
3 participants
Add FavourNestedFunctions rule.
Fixes #638